Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resultType extension method on Symbol #381

Merged
merged 3 commits into from
Sep 28, 2017

Conversation

gabro
Copy link
Collaborator

@gabro gabro commented Sep 28, 2017

This seemed like a useful utility so I extracted it from ExplicitResultTypes

@gabro gabro added the core label Sep 28, 2017
@gabro gabro requested a review from olafurpg September 28, 2017 08:19
for {
symbol <- t.symbol
resultType <- symbol.resultType
} yield assert(resultType.structure == Type.Name("Boolean").structure)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a smarter way to compare trees?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is! import scala.meta.contrib._; q"a.b".isEqual(q"a.b.")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I'll fix it

val defaultDialect =
dialects.Scala212.copy(allowMethodTypes = true, allowTypeLambdas = true)

def resultType(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is up to debate. resultType is general enough, but it's a bit of a stretch when referring to simple val/var

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result type is the correct nomenclature from scalac 👍

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 besides the minor comments

val defaultDialect =
dialects.Scala212.copy(allowMethodTypes = true, allowTypeLambdas = true)

def resultType(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result type is the correct nomenclature from scalac 👍

def resultType(dialect: Dialect): Option[Type] =
denotation.flatMap(denot =>
DenotationOps.resultType(symbol, denot, dialect))
def resultType: Option[Type] = resultType(DenotationOps.defaultDialect)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we only keep this overload and let advanced users with custom dialects call DenotationOps.resultType(customDialect)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me

for {
symbol <- t.symbol
resultType <- symbol.resultType
} yield assert(resultType.structure == Type.Name("Boolean").structure)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is! import scala.meta.contrib._; q"a.b".isEqual(q"a.b.")

@olafurpg
Copy link
Contributor

We should filter out scalafix.internal.* from mima like here https://github.com/scalameta/scalafmt/blob/143bff64f24c4db6b53998a927d317ee0d874bcc/project/Mima.scala#L11

We need to upgrade Mima to 0.1.15

addSbtPlugin("com.typesafe" % "sbt-mima-plugin" % "0.1.14")
which added support for regexes in filters

@gabro
Copy link
Collaborator Author

gabro commented Sep 28, 2017

@olafurpg I've added the mima change in this PR, so that we can test it on the spot. Hope you don't mind

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, it's fine to do the mima changes here

@olafurpg olafurpg merged commit d511c19 into scalacenter:master Sep 28, 2017
@olafurpg olafurpg added this to the v0.5.3 milestone Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants